is this a well defined imagestream with a pre-defined name we agree upon? how do we know which imagestream to look for? or are we assuming there will be only one imagestream, so we just pick that up and use it?

We can keep the parameter ImageStreamTag in <ImageStream_name>:<Tag_name> format.
Multiple ImageStreams can be created in the operator namespace.
By parsing ImageStreamTag we can get hold of which ImageStream to look for and the respective tag.

should we allow multiple custom images to be specified?

We will allow one custom image per CR. It will be easier to maintain the success/failures of each image run.

this will look for an imagestream tag in an imagestream in the namespace of the CR. how do we map it to fetch the imagestream tag from the must-gather-operator namespace?

We can recommend to create all imagestreams in operator namespace only. Operator (in reconcile logic) will only look for imagestreams in operator namespace.
Please let me know if you see issues with this approach.

so operator will resolve the imagestream tag and use that image to create the pod?

yes. Operator will resolve imagestream tag, fetch corresponding image - if valid - create the must-gather pod with that image.

we need to factor in some field for letting users choose some custom arguments for the custom must-gather images that they trigger.

it's mainly so we can conclusively support these two use cases:

1. IIRC, in the past enhancement discussion we wanted to introduce an additionalConfig struct that allows customizing the default platform must-gather into running /usr/bin/gather_audit
2. HyperShift custom must-gather dumping i.e. SREP-1884: Add support for ACM Hosted Control Plane (HCP) must-gathers must-gather-operator#296 (comment) requires the ability to specify arbitrary config value for HostedClusterOptions{Namespace, Name}, etc.

Explicitly for audit or /usr/bin/gather_sudit_logs we have a separate API field in the spec.
https://github.com/openshift/must-gather-operator/blob/master/controllers/mustgather/template.go#L29
https://github.com/openshift/must-gather-operator/blob/master/controllers/mustgather/template.go#L27

HyperShift custom must-gather dumping i.e. openshift/must-gather-operator#296 (comment) requires the ability to specify arbitrary config value for HostedClusterOptions{Namespace, Name}, etc.

yes we can add a new struct additionalConfig for any other custom arguments.
Should we do that as part of the same enhancement or a new one?

Sounds reasonable IMHO to add this in the same enhancement, as I'd assume many custom must-gather images could benefit from custom args.

now with the introduction of imageStreamTag, how is the default platform must-gather image (that we ship/referred to be known in our operator) handled?

also, am assuming we treat the "default platform must-gather image" as an exception and don't change the behavior for it.

Still we need to support the images in https://github.com/openshift/oc/blob/main/pkg/cli/admin/mustgather/mustgather.go#L166 i.e. the custom must-gather images for each operator that is installed on the cluster and that ships a well trusted gather image. It can be very cumbersome for a cluster admin to have an imageStreamTag against each operator image especially when operators are updated the tags/images can change.

also, am assuming we treat the "default platform must-gather image" as an exception and don't change the behavior for it.

imageStreamTag will be an optional parameter. If user does not specify it, operator will fall back to default must-gather image.

Still we need to support the images in https://github.com/openshift/oc/blob/main/pkg/cli/admin/mustgather/mustgather.go#L166 i.e. the custom must-gather images for each operator that is installed on the cluster and that ships a well trusted gather image. It can be very cumbersome for a cluster admin to have an imageStreamTag against each operator image especially when operators are updated the tags/images can change.

There are some ways we can tackle this:

1. For this we can introduce another API field, similar --all-images. If that is set operator will look for the must-gather annotation and create imagestreams for those images instead of an admin.
2. As part of operator initialization/deployment operator scans and discovers these annotations and create separate imagestream or create a single imagestream and add these operator-must-gather images as a unique tag under one imagestream.
   What we will save by this is, admin does not have toc reate imagestream(s) for these operator-must-gather images.

Still we need to support the images in https://github.com/openshift/oc/blob/main/pkg/cli/admin/mustgather/mustgather.go#L166 i.e. the custom must-gather images for each operator that is installed on the cluster and that ships a well trusted gather image. It can be very cumbersome for a cluster admin to have an imageStreamTag against each operator image especially when operators are updated the tags/images can change.

There are some ways we can tackle this:

1. For this we can introduce another API field, similar --all-images. If that is set operator will look for the must-gather annotation and create imagestreams for those images instead of an admin.
2. As part of operator initialization/deployment operator scans and discovers these annotations and create separate imagestream or create a single imagestream and add these operator-must-gather images as a unique tag under one imagestream.
   What we will save by this is, admin does not have toc reate imagestream(s) for these operator-must-gather images.

Can we first find out if there is a need to do this? i.e, do current users of the operator actually require this feature. I would like to start with just supporting collection of must-gather through specifying the custom images explicitly

imageStreamRef with {Name, Tag, ..} would be more appropriate.

or generally, a LocalObjectReference is a good choice for this type of references as this keep future extensibility options open.

imageStreamRef

(eg. our CI release contoller uses imageStreamRef)

Thanks for the suggestion @swghosh. I have included it in the proposal now.

as the intent here was to support arbitrary parameter values, would you think we should opt for an unstructured field instead?

eg. runtime.RawExtension to allow any arbitrary key structs being placed in here.

My understanding was, if we want to add/support any specific functionality from operator side (for which changes are needed in operator space) will go under AdditionalConfig, in this case then I felt structured field would make sense. For custom args anyway we will have args parameter, that can have any value depending the custom image requirements.

Do you feel we should support custom/arbitrary fields under additionalConfig as well? @swghosh

IMHO sounds fair, we can take that approach. But in case we fully want to support the least customisation on the custom gather images, I guess we should atleast provide for

struct{ GatherCommand string, Args []string }

that way we won't need to jump to arbitrary unstructured immediately, yet provide room for enough customisation (args and command is something our oc adm must-gather cli already provides).